fix(plugins): replace dependency marker files with a real satisfaction check#390
Conversation
…n check The .dependencies_installed hash-marker system only tracked "was this exact requirements.txt hashed before" — not whether the packages it names are actually present. That made it fragile (a wiped venv, a manually removed package, or a lost/corrupted marker forces a needless full pip reinstall or, worse, a false skip) and produced dead weight for the ~10 plugins whose requirements.txt is comment-only (they still paid a pip subprocess on first boot before a marker existed). Replace it with requirements_are_satisfied() in plugin_loader.py, which checks each real requirement line against importlib.metadata directly, so install_dependencies() only shells out to pip when something is actually missing or version-mismatched. Drops the marker file entirely: removed all marker read/write sites in plugin_loader.py and store_manager.py, the now-pointless marker-cleanup step in the git-update path, the unused legacy marker implementation in plugin_manager.py, and the already-stale clear_dependency_markers.sh script. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ
|
Warning Review limit reached
Next review available in: 25 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughPlugin dependency installation now parses requirements and checks installed versions before invoking pip. Marker-file tracking and duplicate installers were removed, store updates adjust stashing behavior, and tests cover satisfied dependencies and retry paths. ChangesPlugin dependency installation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | -6 |
| Duplication | -2 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/plugin_system/store_manager.py (1)
1900-1923: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated preflight-check pattern with
plugin_loader.install_dependencies.The
requirements_has_real_deps/requirements_are_satisfiedguard-and-log sequence here is nearly identical to the one insrc/plugin_system/plugin_loader.py'sinstall_dependencies(lines 261-273). Consider extracting a small shared helper (e.g.should_skip_pip_install(requirements_file, label, logger)) that both call sites can reuse, since the two pip-invocation strategies (subprocess vsinstall_requirements_file) can remain separate.♻️ Possible shared helper
def should_skip_pip_install(requirements_file: str, label: str, logger) -> bool: if not requirements_has_real_deps(requirements_file): logger.debug("requirements.txt for %s has no real dependencies, skipping pip", label) return True if requirements_are_satisfied(requirements_file): logger.debug("Dependencies for %s already satisfied, skipping pip", label) return True return False🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/plugin_system/store_manager.py` around lines 1900 - 1923, Extract the duplicated requirements preflight checks from _install_dependencies and plugin_loader.install_dependencies into a shared should_skip_pip_install helper, reusing requirements_has_real_deps and requirements_are_satisfied with consistent debug logging. Update both callers to invoke the helper while preserving their existing, separate pip installation strategies and the missing-file handling in _install_dependencies.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/plugin_system/plugin_loader.py`:
- Around line 26-93: Address CodeQL’s uncontrolled-path warnings in
requirements_has_real_deps and requirements_are_satisfied by making each
function explicitly validate or sanitize requirements_file before calling
open(). Reuse the module’s established realpath/basename containment approach,
or change the functions to accept a prevalidated Path type and enforce the
allowed directory boundary internally; update their callers accordingly while
preserving existing error behavior.
---
Nitpick comments:
In `@src/plugin_system/store_manager.py`:
- Around line 1900-1923: Extract the duplicated requirements preflight checks
from _install_dependencies and plugin_loader.install_dependencies into a shared
should_skip_pip_install helper, reusing requirements_has_real_deps and
requirements_are_satisfied with consistent debug logging. Update both callers to
invoke the helper while preserving their existing, separate pip installation
strategies and the missing-file handling in _install_dependencies.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ada0a19e-e8d2-4b39-9438-6e21851f72ac
📒 Files selected for processing (7)
requirements.txtscripts/clear_dependency_markers.shsrc/plugin_system/plugin_loader.pysrc/plugin_system/plugin_manager.pysrc/plugin_system/store_manager.pytest/test_plugin_loader.pytest/test_plugin_system.py
💤 Files with no reviewable changes (2)
- src/plugin_system/plugin_manager.py
- scripts/clear_dependency_markers.sh
…ecks CodeQL flagged 2 new high-severity "uncontrolled data used in path expression" alerts at the open() calls inside this PR's new requirements_has_real_deps()/requirements_are_satisfied() -- both are reachable from paths that were never run through the basename+trusted-base sanitiser this codebase already uses elsewhere: - PluginLoader.install_dependencies() only applied that sanitiser when its optional plugins_dir argument was actually passed; the "no plugins_dir" branch trusted plugin_dir_real directly. Made plugins_dir required (not Optional) so that branch can't exist, and added an explicit guard in load_plugin() so install_deps=True without a plugins_dir fails loudly instead of silently. Production's only real caller (PluginManager) always passes plugins_dir already; the harness/dev-server/render-plugin callers all use install_deps=False and are unaffected. - StoreManager._install_dependencies() never sanitised plugin_path at all, and its call sites ultimately derive that path from a plugin's own manifest.json "id" field (install_plugin_from_url) -- a malicious plugin could otherwise point requirements_file outside plugins_dir. Applied the same os.path.basename()-based containment pattern PluginLoader already uses (and that CodeQL recognises as a real sanitiser). Added test_install_dependencies_requires_plugins_dir and test_install_dependencies_rejects_path_outside_plugins_dir to lock in the actual security property, not just quiet the scanner. Verified: all 20 tests in test_plugin_loader.py pass, plus the PR's existing test plan (test_plugin_system.py, test_store_manager_caches.py: 53 passed) and the full CI plugin-safety suite (test_harness.py, test_visual_rendering.py, test_plugin_matrix.py: 52 passed, 2 pre-existing skips) all still pass.
…tion check The previous commit's os.path.basename() + os.path.join() pattern (which a pre-existing code comment claimed CodeQL recognises as a sanitiser) did not actually clear the alert -- the next CodeQL run still flagged the same 2 sink lines, plus a new one at the os.path.join() call itself. Taking a substring of tainted data apparently isn't treated as a barrier by this query, whatever the comment assumed. Replaced it with find_trusted_subdir(): enumerate the trusted plugins_dir via os.scandir() and only use a name that scandir itself produced, matched by equality against the caller's requested name. The path is then built from that enumerated entry, not from the caller's string -- a value sourced from iterating a trusted, non-tainted directory carries no taint regardless of what it happens to equal, which is a stronger and more conventional allowlist-style barrier than string-stripping. Applied identically in both PluginLoader.install_dependencies() and StoreManager._install_dependencies(), sharing one implementation. Re-verified: all 65 tests across test_plugin_loader.py (20, including the 2 new security regression tests), test_store_manager_caches.py (35), test_plugin_system.py (10) pass, plus the full CI plugin-safety suite (test_harness.py/test_visual_rendering.py/test_plugin_matrix.py: 52 passed, 2 pre-existing skips).
…re logging CodeQL flagged 3 clear-text-logging-of-secrets alerts in install_requirements_file() (src/common/permission_utils.py:353,360,371). Pre-existing on main, unrelated to this PR's own diff, but now visible since the path-injection alerts that previously took priority in the annotation list are fixed. The underlying risk is real: pip can echo a private index URL's embedded basic-auth credentials (from a requirements.txt --index-url line or PIP_INDEX_URL) back verbatim in its own stderr/stdout on failure, and this function both logs that output directly and returns it to callers -- store_manager.py's _install_dependencies() logs result.stderr from this same function too. Added _redact_url_credentials(), applied immediately after each of the two subprocess.run() calls (mutating result.stderr/stdout in place) rather than patching each log call site individually. This closes the leak at the source: every downstream use -- the three flagged log lines, the "note" string embedded in the returned stdout, and store_manager.py's own logging of the returned result -- gets the redacted text for free. Verified the fixed-phrase "denied" check (`"a password is required" in result.stderr`) is unaffected, since URL syntax and those phrases don't overlap -- covered explicitly by test_does_not_touch_denied_check_phrases. Added test/test_permission_utils.py (6 tests) covering the redaction helper directly and both subprocess.run() call sites (the sudo-wrapper branch, which this repo's scripts/fix_perms/safe_pip_install.sh makes live, and the no-wrapper fallback branch). All pass.
The previous commit's redaction (mutating result.stderr/stdout right after each subprocess.run()) didn't clear CodeQL's clear-text-logging alerts -- same lesson as the path-injection fix earlier in this PR: a static analyzer can't tell "this value was already sanitised two lines up" from "this is still the raw tainted value" just by looking at a single log call in isolation, so it conservatively keeps flagging it regardless of what the redaction function actually does. Removed all dynamic interpolation (req_file, result.stderr) from the 3 flagged logger.warning() calls entirely, replacing them with fixed messages plus (for the one that had it) result.returncode, which is a plain int with no possible taint. The full redacted detail is still available where it actually matters -- in the returned CompletedProcess.stderr/stdout and the "note" text -- just not duplicated into a log line a scanner has to reason about in isolation. Re-verified: all 6 test_permission_utils.py tests still pass (they assert on the returned result, not log call arguments), plus the full test_plugin_loader.py/test_store_manager_caches.py/test_plugin_system.py suite (71 passed, 1 pre-existing deselect, 4 subtests).
Summary
.dependencies_installedhash-marker system only tracked "was this exactrequirements.txthashed before," not whether the packages it names are actually installed — fragile against wiped venvs, manually removed packages, or lost/corrupted markers, and dead weight for the ~10 plugins whoserequirements.txtis comment-only.requirements_are_satisfied()inplugin_loader.py, which checks each real requirement line againstimportlib.metadatadirectly, soinstall_dependencies()only shells out to pip when something is actually missing or version-mismatched.plugin_loader.py/store_manager.py, the now-pointless marker-cleanup step in the git-update path, the unused legacy marker implementation inplugin_manager.py, and the already-stalescripts/clear_dependency_markers.sh.packagingas an explicit top-level dependency (was only a transitive dep ofpytestbefore).Test plan
./venv/bin/python3 -m pytest test/test_plugin_loader.py test/test_store_manager_caches.py -q— 53 passed./venv/bin/python3 -m pytest test/test_plugin_system.py -q --deselect test/test_plugin_system.py::TestPluginHealth::test_circuit_breaker— 10 passed, 1 deselected (pre-existing unrelated failure — stale mock signature in a cache_manager test, confirmed failing in isolation on this branch's base too)pytest) skips the pip subprocess entirelyrequirements_are_satisfied()against real pluginrequirements.txtfiles (vacuous and real-deps cases) and edge cases (unmet packages, impossible version specs,-rpip-option lines)🤖 Generated with Claude Code
https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ
Summary by CodeRabbit
New Features
Bug Fixes